-
Notifications
You must be signed in to change notification settings - Fork 903
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Windows & X11: Window::set_resizable #558
Windows & X11: Window::set_resizable #558
Conversation
Gah, I was looking forward to nagging you about needing to handle that, but it seems you already knew. The line I'd prepared was about how the fact that this sets the min/max size should be an implementation detail that's encapsulated by the API. That state should be stored in
If I were you, I'd do all of this in one PR, or just take the approach I used in #553 and don't add the top-level Since this seems to be mostly complete, I'm not against including it in 0.15.1, though it also doesn't really matter, since 0.16.0 will be released quite soon afterwards (I just want to get some regression fixes out instead of gating them behind an API migration). |
CHANGELOG.md
Outdated
@@ -3,6 +3,7 @@ | |||
- On X11, the `Moved` event is no longer sent when the window is resized without changing position. | |||
- `MouseCursor` and `CursorState` now implement `Default`. | |||
- `WindowBuilder::with_resizable` implemented for Windows, X11, and macOS. | |||
- `Window::set_resizable` implemented for MacOS, X11, and Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The correct stylization is "macOS", and it also reads a little easier if you use the same ordering as in the previous line.
examples/resizable.rs
Outdated
} | ||
_ => winit::ControlFlow::Continue, | ||
}, | ||
_ => winit::ControlFlow::Continue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an official winit example™, it would be good if you refactored this to not repeat winit::ControlFlow::Continue
, i.e. https://github.com/tomaka/winit/blob/master/examples/handling_close.rs
src/lib.rs
Outdated
@@ -422,7 +422,7 @@ pub struct WindowAttributes { | |||
/// The default is `None`. | |||
pub max_dimensions: Option<(u32, u32)>, | |||
|
|||
/// [Windows & X11 only] Whether the window is resizable or not | |||
/// [Windows, MacOS, and X11 only] Whether the window is resizable or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you followed the precedent set by [iOS only]
, but you should know two things about that:
- The
[iOS only]
doc comment isn't accurate anymore... - That was added a long time ago, and back before winit had someone barking about style, so it's not necessarily something you should replicate. Sticking to the conventions established in other areas of the documentation is probably better.
src/platform/linux/mod.rs
Outdated
match self { | ||
&Window::X(ref w) => w.set_resizable(resizable), | ||
// &Window::Wayland(ref w) => w.set_resizable(resizable), | ||
_ => {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to favor _ => ()
, but more importantly, it's more appropriate to stub this as unimplemented!()
, by decree of tomaka.
The idea is that it makes people more likely to fix it... in my experience, all it does is make people more likely to yell at us, though that does make me more likely to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've released version 0.2.2 of smithay-client-toolkit which adds a set_resizable
method to its Window
type, that does what we need for wayland support. So, fixing should be pretty trivial, I can do this once this is merged, as soon as I secure the 5 minutes it'll take.
src/platform/linux/x11/window.rs
Outdated
unsafe { | ||
self.update_normal_hints(|size_hints| { | ||
if resizable { | ||
(*size_hints).flags &= !ffi::PMinSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can cut the number of flag changes in half in this function.
src/platform/windows/window.rs
Outdated
winuser::GetWindowLongW(self.window.0, winuser::GWL_STYLE) | ||
}; | ||
if resizable { | ||
style |= winuser::WS_SIZEBOX as i32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer you use the winapi LONG
type instead of i32
.
src/platform/windows/window.rs
Outdated
if resizable { | ||
style |= winuser::WS_SIZEBOX as i32; | ||
} else { | ||
style &= !winuser::WS_SIZEBOX as i32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the precedence here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The precedence for removing the WS_SIZEBOX
when setting resizable to false? I guess I'm not 100% sure what you are referring to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol. The precedence is LONG
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just wanted to make sure it was zero-extending before flipping the bits. Or rather, I wanted to make sure you thought about that.
src/platform/windows/window.rs
Outdated
@@ -239,6 +239,33 @@ impl Window { | |||
} | |||
} | |||
} | |||
|
|||
/// See the docs in the crate root file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to add this comment, since #548 completely removes them... they don't seem to be at all helpful, especially since these are doc comments on private methods.
More importantly, what happens when you use this method while the window is fullscreen?
cbeaa71
to
4b280ef
Compare
4b280ef
to
dd1022a
Compare
I addressed style changes and X11 remembering the window min/max size. I haven't addressed or tested setting resizable while in fullscreen. |
src/window.rs
Outdated
/// ## Platform-specific | ||
/// | ||
/// This only has an effect on Windows, X11, and macOS. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You left the block comment open/close in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, good catch. I'm not sure I would have ever seen that as a code reviewer.
For whatever reason, |
This description doesn't seem entirely accurate; from what I'm seeing Xfwm stops picking up on changes whenever min/max are set to values that make it unresizable (even when after mapping): https://mail.xfce.org/pipermail/xfce/2015-August/034641.html Note that my phrasing of "values that make it unresizable" rather than "the same value" was deliberate; I tried to outsmart it by adding 1 to the min size and subtracting 1 from the max size, but the problem was still present. (It would've been a bad hack to use anyway, since I'm sure there are some things that become quite unhappy if min>max.) So, once logging is added, I'll just add a |
As predicted, things on Windows don't quite work right with fullscreen mode. Creating a window
There don't seem to be any other problems, though. |
So, the situation with Xfwm is worse than I thought, since it completely breaks the DPI scaling that will be introduced in #548. I think the simplest thing to do is to just have these methods no-op if you're running Xfwm. While it's not the most elegant thing, the only alternative is to add a scary notice to the docs and expose I'll add that check myself, it's just I want to make sure there's agreement that it's reasonable. I'd also like to add some documentation stating that using these methods doesn't exempt one from handling |
So I made it so it can go fullscreen and change the resizability of the restored window on Windows. I'm not particularly fond of the duplication of function in the I'll mention that when going from window to fullscreen and back to window, min and max window sizes are forgotten. |
I'm guessing it already had that problem (this isn't a regression)? |
Oh, actually, it turns out I removed the min/max from my example and forgot it. So it is definitely remembering the min/max of the window. https://gist.github.com/dannyfritz/6eb8e7718b1cb3871e11fdefa32d0a14 But... the fullscreen is... interesting. It only gets to the size of the max dimensions. |
And testing the example without |
What's supposed to happen here isn't well-defined, which is to say, no one's actually thought about it much. Though, I think we probably all expect setting a window to fullscreen to set it to fullscreen, so that should probably take precedence over the max size. Maybe I'll remember to fix that when I rebase #548, since that also adds logic for determining when the window is entering fullscreen. Anyway, thanks for all the work on this! We should be good to merge now, right after I make the changes I mentioned. |
That's great to hear. 😄 You have been both very knowledgeable and helpful throughout the process. |
Aw, thanks so much! |
CHANGELOG.md
if knowledge of this change could be valuable to usersRelated to #540